-
Notifications
You must be signed in to change notification settings - Fork 160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-4968: server-spring - Close query result on pre-render exception #4969
GH-4968: server-spring - Close query result on pre-render exception #4969
Conversation
queryResponse.close(); | ||
} | ||
} catch (Exception qre) { | ||
logger.warn("Query response closing error", qre); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: I thought it might be sensible here to log the result-close failure so the client still receives the original failure reason. Is that the right approach? And: Would it make sense to do the same for repositoryCon
below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is an improvement and can be merged as is.
It looks like the integration test failure is unrelated (in that this from my understanding doesn't use
|
I'm rerunning that GitHub action to see if it is just temperamental. I haven't seen it being flaky before though. |
// only close the response & connection when an exception occurs. Otherwise, the QueryResultView will take | ||
// care of closing it. | ||
try { | ||
if (queryResponse != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can simplify your code down to an instanceOf AutoClosable check here without having to introduce the Boolean query stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fb01e2a
(to be squashed before merge)
Test is still failing. I suspect it's related to the Boolean query changes you've introduced. I added a proposal to your code for a simple instanceOf AutoCloseable check instead. |
I just realised that I'm probably doing the whole "add more commits during review" thing wrong: Internally we use What's your preference?
|
We typically rebase and squash locally then force push the branch. When we merge here on GitHub we try to only use regular merge since either of squash/rebase changes the author on the commits. If you can squash your commits into one and force push it I’d be grateful. |
…r exception - Previously between getting the query response and rendering it to the desired output an exception could occur (e.g. due to unsupported result format) which would leave the result un-closed. This in turn would sometimes trigger an exception on closing the associated repo connection.
b365276
to
f9ed099
Compare
Squashed |
Ah wait - should the target branch also be |
Since this was now a pure bug fix I went ahead and merged it into main instead of develop. We regularly merge main into develop to keep things in sync. |
GitHub issue resolved: #4968
Briefly describe the changes proposed in this PR:
AbstractQueryRequestHandler
class.PR Author Checklist (see the contributor guidelines for more details):
mvn process-resources
to format from the command line)